Lock Improvements #129
Lock Improvements #129
Conversation
Having the lock be an argument makes it easier to implement different types of locks, since we won't need to subclass from Scheduler and instead can implement smaller classes.
This allows for multiple scheduling processes to run concurrently, but only one of them will run jobs at any given time.
| @@ -65,7 +65,7 @@ def delete_unscheduled | |||
|
|
|||
| @mutex.synchronize { | |||
|
|
|||
| @array.delete_if { |j| j.next_time.nil? || j.unscheduled_at } | |||
| @array.delete_if { |j| !j.next_time || j.unscheduled_at } | |||
There was a problem hiding this comment.
@jmettraux am I right to think this is a bug?
There was a problem hiding this comment.
So if we have:
job = proc { }
scheduler = Rufus::Scheduler.new
scheduler.schedule_in(0, job)Once the job is triggered, its @next_time value will be false (since it'll only run once). Shouldn't it get deleted from the jobs array at that point?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, that makes sense.
My example is what happens when confirm_lock returns true, so the job triggers but it doesn't run its block:
callable = proc { }
scheduler = Rufus::Scheduler.new
scheduler.pause # let's do some setup first
# jobs will still be scheduled and triggered, but the trigger method will return early
def scheduler.confirm_lock
false
end
job = scheduler.schedule_in(0, callable)
job.next_time # => Time object
# let's trigger jobs again
scheduler.resume
sleep 1
job.next_time # => false
def scheduler.confirm_lock
true
end
# job is still not run, but stays in scheduler.jobsSo my question: what does it mean when @next_time is false? Doesn't seem like the job is ever run.
There was a problem hiding this comment.
@next_time == false, means the job was about to be triggered (pre trigger) but didn't. @next_time == nil is when post trigger.
I'll investigate what you found in gh-130
Thanks a lot!
|
Hello, so this pull request comprises two things, a refactor and a bug fix. The refactor work is elegant. I don't see a spec for
or for
I don't see code that prevents scheduled jobs from occurring in one of those "locked out" scheduler instances. Thanks for your time. |
as surfaced in discussion at gh-129
|
#131 continues this work. |
This code goes along with #128
It refactors how
Rufus::Schedulerhandles locks (no more need for subclasses!), but more importantly it allows multiple schedulers to run concurrently, with only one of them running jobs at any given time.Additionally, this fixes a seeming bug with
JobArray#delete_unscheduled. A job's@next_timevalue can benilorfalseif it never intends to run again, but we only checked for anilvalue. With the fix, jobs that are scheduled usingscheduled_in, for example, will get pruned.